-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(remap): compile-time program result type checking #4902
Conversation
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Previously, accessing a path was a fallible operation, and the remap condition implementation is configured to allow a script to fail (in which case the condition returns `false`). Paths queries have been changed to never return an error, but return `None` if the path doesn't exist. This means the condition check now also has to accept none values, and consider them `false`. This is a temporary change, as optional values are going to be removed from the Remap language. Non-existing paths will instead resolve to `null`. Signed-off-by: Jean Mertz <git@jeanmertz.com>
3a7cd48
to
1c7be2a
Compare
Signed-off-by: Jean Mertz <git@jeanmertz.com>
I do really like this. It is a great start, and gives us a lot of potential to take things much further. Thinking ahead to template fields, it would be really useful if we could limit template fields to just return Strings (or at least reject Obviously, path schemas would alleviate this issue. But in the absense of those, I think it would be useful to specify a type constraint of |
Yeah, I like that. I'll make a note and tackle that in a follow-up PR, so that we can use it in the templating work we're doing 👍 |
Merging this, as @FungusHumungus has approved it, and there are quite a few PRs dependent on this one being merged. I messaged @jszwedko that I'd be happy to receive a post-merge review. I'll tackle any review comments in a follow-up PR, if needed. |
…4902) Signed-off-by: Brian Menges <brian.menges@anaplan.com>
This PR implements RFC #4862.
This PR is best reviewed *per commit*. While the full diff is rather large, most of that comes from numerous tests.
User-Level Changes
As for the last point, the one place this currently invalidates certain Remap scripts is if you use remap as a condition.
Take for example:
The
Condition
trait we use only allows returning boolean values (for performance reasons), so the only thing we could do was to accept the abovestarts_when
condition, but always returnfalse
because1337
is not a boolean.After this PR, we can check the return value at boot-time, which allows us to return an error and prevent the above example config from being valid, pointing out that
starts_when.source
needs to resolve to a boolean. A user can use any of the available functions or operations to make sure the return value is a boolean:to_bool(1337)
— true1337 >= 0
— truecontains(to_string(1337), substring = "133")
— trueThis PR has no impact on the Remap transform, as the return value of the script isn't relevant to the outcome of the transform (the script is used to manipulate events, not to resolve to a final value).
Program Type Check
When compiling a Remap script, the callee informs the program which results it expects. The following constraints can be defined:
For example:
The above constraint requires the program to be infallible, and return a concrete value type of any kind.
Exact value constraints can be defined as
Any
,Exact(Boolean)
. Multiple value kinds are defined asOneOf(vec![Integer, Float])
.The program returns an error at compile-time if the result does not match the expectation, e.g.:
or
The
fallible
check applies to all (root) expressions in a program (since any failing expression will fail the entire program), whereas theoptional
andconstraint
checks only apply to the last expression (the one that resolves to the final value of the program).Expression Type Definitions
To make the above work, every expression now implements:
There are several rules that apply:
.foo.bar
) resolve toAny
(because we don't use any schemas currently, but that could change with Log schemas #3910), unless a path is assigned a value, in which case the path gets whatever constraint the value has (e.g..foo = true
will type check.foo
as aBoolean
).$foo = true; $foo = "bar"; $foo
type-checks to a string)block
expression delegates type checking to the final expression in the block, and an if-else statement merges the if and else type checks together)type_def
, and have to make sure the contract between implementation andTypeDef
holds.Future Work
While we're currently only using this information on a program-level, we can (and probably will) use this on an expression-level, to reject certain programs.
For example:
The above will fail at runtime because
"foo"
is of kindString
, whereas the condition in an if-statement has to resolve to a boolean.In a future PR, we can (at compile-time) ask the constraints of
$foo
, and know that it'll never resolve to a boolean, and thus return a compile-time error. To resolve this, one could instead writeif to_bool($foo)
. The same applies to certain operators (e.g."foo" > 2
would fail to compile).